[No QA] Fix Fullstory masking logic on App - #73456
danieldoglas merged 37 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
|
|
Regarding my own comment of two issues reported here.
Pushed a commit to mask only the search items that are supposed to be, just like the LHN.
I did some investigation here and looks like it's some issue/delay in Fullstory session to be able to recognise some items as unmasked, because I checked them by myself and all the ones that are supposed to be unmasked are receiving the correct prop. |
|
Bump @jjcoffee @danieldoglas |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
jjcoffee
left a comment
There was a problem hiding this comment.
Changes look good and tests well! I'm not sure if this should be tagged No QA, since QA probably can't test fullstory?
|
@fabioh8010 Are the failing tests coming from main? |
|
|
@jjcoffee Fixed them by merging with |
|
@fabioh8010 Looks like lint is still failing? |
The ESLint errors failing are about:
Do you think it's something we can skip in this PR? I could fix Deprecation of assets and useOnyx ones, but the PR size will increase a bit I think. |
|
I'll leave it up to @danieldoglas, but I agree it's out of scope for this PR. |
|
Hello! I'm back from OOO, @fabioh8010 can you do a last merge here so we can check if the test failures will stop? |
|
Cool, talked about this internally and I think we're good to move forward with merging it without addressing those lint issues. Mostly they were not caused by this PR, and this is a quite large one. |
|
@danieldoglas looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
The lint issues that existed were unrelated to this PR, they were appearing just because we were touching those files. Removing emergency label. |
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.2.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
Explanation of Change
This PR aligns the Fullstory masking logic with expected behavior as discussed with the client.
Fixed Issues
$ #72242
PROPOSAL:
Tests
What should be masked
https://app.fullstory.com/ui/o-1WN56P-na1/session/8635679420741252980:8470879015830778750!6038200911789821161
https://app.fullstory.com/ui/o-1WN56P-na1/session/6847252140187870127:8446063933385999091!8465590110379671145
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop